Fix #8306: Ensure the inliner can elide effectively pure case class applications in various situations #8348
Conversation
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Add" instead of "Added")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
|
re: failed community build, I'm not sure what to make of the error. I re-ran the builds locally and all that failed were a couple of tests in os-lib that seem to be due to me having not made bash / python available... but also, the number of failed tests was different on my machine and on CI. Weird. EDIT: I did slightly more digging and I'm seeing a lot of |
|
https://dotty-ci.epfl.ch/lampepfl/dotty/4528 – passes We're currently migrating to a new CI, this error is related to the migration process. FTTB we ignore the GitHub Actions (new, failing) CI when merging PRs, only the Drone CI counts. os-lib tests are flaky. On different systems, different tests fail. |
|
@fhackett I think everything is fixed on the CI side, please rebase to run it again. |
|
@fhackett Do you have time to try and rebase? |
|
@bishabosha Sorry, yes. It got lost in my TODO list as I got into other things since this PR was posted. Rebase coming up soon. |
…ass applications in various situations This is a collection of related changes, all to ensure that the inliner is capable of properly eliding case class constructions in various situations. Specific intended changes: - allow NewInstance.unapply to look past type ascriptions - change all purity checks to consider, for the inliner only, case class applications (when the apply method is synthetic and the case class has no constructor body) pure, or "elideable", despite the fact that the operation is almost always idempotent in the general case. - make reduceInlineMatch's reduceSubPatterns procedure set the defTree for the accessor projections it generates, so nested Unapply patterns can look in defTree to find and possible inline the appropriate subtree of the scrutinee expression - make the typedSelect override able to reduce projections without a corresponding inline match, so that if inlining produces SomeCaseClass(x=3).x the case class construction has a change to be elided (only in an inline context) This commit also includes a series of test cases which use compiletime.show to pretty-print the resulting inlined AST of a series of inline expansions. To avoid cases where a malformed AST pretty-prints but causes bad codegen, the computed run-time value is also printed. One last thing I tested manually was that the bytecode matched the pretty-printing. Mid-work on this commit I found that the pretty printer was hiding several cases where redundant code was left in but not showing up when pretty-printed. This is partially tested by the case where full inlining is not possible, which would also include a redundant binding for the scrutinee if that issue were still present.
fhackett
force-pushed
the
fhackett-fix-8306
branch
from
fc046fb to
ae336b6
Compare
|
@bishabosha and rebased. Some thoughts since I originally posted this PR: I tried using these changes in larger-scale tests and while they work quite well there does remain some messiness to deal with going forward (more dead code left in, etc). I've shifted focus so I won't have a lot of time to look into it myself for a while, but I wanted to at least document it. |
|
Awesome, thanks |
|
We should add this to the list of things that will not work under separate compilation. Before it was OK to change the body of a case class from being pure to not being pure. Now it's not. |
|
should this be reverted? |
|
I don't know. It depends on what people are willing to live with. But maybe this change is qualitatively worse, because now, even changing the implementation of non-inline stuff can break stuff. Before only changing the implementation of things explicitly marked |
This is a collection of related changes, all to ensure that the inliner is capable of properly eliding case class constructions in various situations.
While I have made sure to avoid breaking anything, these changes do noticeably change Dotty's behaviour in common situations. Feedback on the direction of these changes is appreciated. See the issue for some more rationale.
Specific intended changes:
This PR also includes a series of test cases which use compiletime.show to pretty-print the resulting inlined AST of a series of inline expansions. To avoid cases where a malformed AST pretty-prints but causes bad codegen, the computed run-time value is also printed.
One last thing I tested manually was that the bytecode matched the pretty-printing. Mid-work on this commit I found that the pretty printer was hiding several cases where redundant code was left in but not showing up when pretty-printed. This is partially tested by the case where full inlining is not possible, which would also include a redundant binding for the scrutinee if that issue were still present.